-
Notifications
You must be signed in to change notification settings - Fork 655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor align #1136
Refactor align #1136
Conversation
b358b78
to
01bf8b2
Compare
@mtiberti @wouterboomsma I see that encore also uses the |
Ok So i'm think I'm done with this. All the internal references to the old keyword have been replaced and appropriate deprecation messages are raised right now when the old one is used. Only the encore module still use the old keyword. I haven't looked yet at how hard it would be to change them as well. |
I haven't looked into actually using arbitrary weights in the tests. I will still do that. |
c8a6ecd
to
1c7c671
Compare
@MDAnalysis/coredevs can anyone see why the travis build fails. For some reason nosetest doesn't exit with 0. |
warnings.simplefilter('always') | ||
RMSD = MDAnalysis.analysis.rms.RMSD(self.universe, select='name CA', | ||
step=49, mass_weighted=True).run() | ||
assert(len(warn), 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis CI build seems to whine about this with testsuite/MDAnalysisTests/analysis/test_rms.py:203: [W0199(assert-on-tuple), TestRMSD.test_mass_weighted_and_save_deprecated] Assert called on a 2-uple. Did you mean 'assert x,y'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. I meant assert_equal
8c324e7
to
a07fcae
Compare
OK I'm done now this PR. Someone could please review this. Their is still the the |
How can I find out in the coverage API which lines I have missed now with the changes here. |
https://coveralls.io/builds/9478683 this shows the current coverage for this commit. coveralls doesn't let you see the delta in lines caused by this PR though, (I think codecov does though) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking clean. Thanks for updating RMSF!
See comments/questions.
I didn't have time to review the tests.
|
||
Returns | ||
------- | ||
old_rmsd | ||
RMSD before spatial alignment | ||
new_rmsd | ||
RMSD after spatial alignment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the description?
package/MDAnalysis/analysis/align.py
Outdated
@@ -404,6 +405,9 @@ def alignto(mobile, reference, select="all", mass_weighted=False, | |||
Uses :func:`get_matching_atoms` to work with incomplete selections | |||
and new *strict* keyword. The new default is to be lenient whereas | |||
the old behavior was the equivalent of *strict* = ``True``. | |||
|
|||
.. versionchanged:: 0.16.0 | |||
new general 'weights' kwarg replace mass_weights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add deprecation for mass_weighted
kw
package/MDAnalysis/analysis/align.py
Outdated
When using 2. or 3. with *sel1* and *sel2* then these selections can | ||
also each be a list of selection strings (to generate a AtomGroup with | ||
defined atom order as described under :ref:`ordered-selections-label`). | ||
mass_weighted : boolean, optional, deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
below you wrote this as `(deprecated) – consistency?
@@ -173,41 +173,34 @@ def shrinkage_covariance_estimator( coordinates, | |||
def covariance_matrix(ensemble, | |||
selection="name CA", | |||
estimator=shrinkage_covariance_estimator, | |||
mass_weighted=True, | |||
weights=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "mass"
to retain the previous default behavior.
req_len = ensemble.select_atoms(selection).n_atoms | ||
else: | ||
req_len = ensemble.atoms.n_atoms | ||
if req_len != len(weights): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this going to fail when weights == None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code path can only be reached if if weights
above evaluates to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
package/MDAnalysis/analysis/rms.py
Outdated
relative_weights = relative_weights.astype(np.float64) | ||
else: | ||
relative_weights = None | ||
|
||
# superposition only works if structures are centered | ||
if center or superposition: | ||
# make copies (do not change the user data!) | ||
# weights=None is equivalent to all weights 1 | ||
a = a - np.average(a, axis=0, weights=weights) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your PR but nicer would be
a -= np.average(a, axis=0, weights=weights)
package/MDAnalysis/analysis/rms.py
Outdated
@@ -335,8 +330,10 @@ def __init__(self, atomgroup, reference=None, select='all', | |||
|
|||
filename : str (optional) | |||
write RSMD into file file :meth:`RMSD.save` | |||
mass_weighted : bool (optional) | |||
mass_weighted : bool deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(deprecated) ?
self._rmsf = rmsf | ||
|
||
@property | ||
def rmsf(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having removed RMSF.rmsf
should be mentioned in a versionchanged 0.16.0 doc, together with the transition to standard analysis API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't removed. It can be read the same way as before. Just that you can now also write to the value. So there isn't a change for the user. We handle all analysis classes right now that computed values can also be overwritten. So this is just making it behave more like the rest of MDAnalysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
The final changes of this depend on #1145 to update encore as well to the new keyword. |
@kain88-de , I had some minor comments on the docs (please have a look) and the question of default values, to which you replied #1136 (comment)
Now that #1145 is done, what should the defaults for mass weighting be? |
@orbeckst thanks for your comments. The default that I changed was to follow the function docs, if I remember correct. The default value for encore is a good question. @wouterboomsma and @mtiberti what weights would you prefer for encore as default masses or none? I'd appreciate input from you anyway with regard to the new feature to choose arbitrary weights. |
@kain88-de Very nice work on supporting arbitrary weights. It should be easy to support this in the encore front-end as well. I think @mtiberti will just look into this. Regarding the default values, the current default is |
simplify weights handling
This new general weights kwarg can take an mass argument or arbitrary weights to allow new fitting algorithms.
This also fixes a centering bug. Previous the center of mass was used independent of the used weights. Now we center with the chosen weights. This means results might change!
The old results have been wrong due to a centering bug in the RMSD code
This changes the methods in encore to the new arbitrary weights system. The last one conf_distance_matrix does need more refactoring work to simplify the code before I change it as well.
a07fcae
to
cffa207
Compare
This replaces the old mass only weights with general weights
Superseeded by #1197 |
It stills needs some checking and tests. I would like go get this into 0.16.0 though. The RMSD class has been refactored this cycle and I'm currently not sure if the bug has been present as well in 0.15.0
Changes made in this Pull Request:
PR Checklist
[ ] Issue raised/referenced?mass_weighted